Skip to content

feat(appkit): add testing kit - #530

Open
IamGalymzhan wants to merge 37 commits into
mainfrom
feat/testing-kit
Open

feat(appkit): add testing kit#530
IamGalymzhan wants to merge 37 commits into
mainfrom
feat/testing-kit

Conversation

@IamGalymzhan

@IamGalymzhan IamGalymzhan commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Testing Kit

  1. Made PluginContext telemetry injectable — the one production edit. It now takes an optional { telemetry }, defaulting to the real provider for testing.
  2. Created the @databricks/appkit/testing package — moved the fixtures out of tools/test-helpers.ts into packages/appkit/src/testing/
  3. Added the two headline helpers — createTestPluginContext() (wraps the real PluginContext with fake tool providers) and expectStream(...).toEmit(...).
  4. Shipped it to users + added docs page (docs/development/testing.md), a template example test.
  5. Added new tests for analytics and genie plugins using this kit

The testing kit needs to construct a real PluginContext without a live
OpenTelemetry pipeline. Add an optional constructor dependency for the
telemetry provider, defaulting to the shared "plugin-context" provider so
the production path is unchanged. This is the single production edit
required to wrap the real class in tests rather than reimplementing it.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Wire the testing kit as a published subpath and prove it against the first
of the two hand-rolled context stubs (the design gate):

- Add ./testing to both exports maps (dev + publishConfig) following the
  ./type-generator shape, add src/testing/index.ts to the tsdown entry, and
  declare vitest as an optional peerDependency. Build passes attw + publint;
  dist/testing/{index,mock-plugin-context,expect-stream,fixtures}.{js,d.ts}
  are emitted and vitest stays external to the main entry.
- Migrate dispatch-tool-call.test.ts: replace (plugin as any).context =
  { executeTool } with mockPluginContext. executeTool is now the REAL method,
  so the forwarded toolCallTimeoutMs is asserted through actual signal
  composition, the on-behalf-of (asUser) path is verified, and a new test
  proves the forwarded timeout actually aborts a slow toolkit tool end-to-end.

This is the primary win from the plan: executeTool's OBO and timeout paths
gain real assertions instead of a stub that proved nothing.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…Context

Replace the second and final hand-rolled stub — (plugin as any).context =
{ addRoute } — with the real PluginContext from mockPluginContext. The kit's
route recorder captures raw handlers, so the alias assertion (both
/invocations and /responses mount the same handler reference) holds against
the real class, where forwardAsyncErrors wrapping would otherwise break
reference identity.

Both context stubs the plan identified are now migrated.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
- Add docs/docs/development/testing.md covering mockPluginContext(),
  expectStream(), and the fixture helpers, with a full end-to-end example.
  Cross-links to local-development, custom-plugins, and execution-context.
- Add template/server/example.test.ts: a self-contained, plugin-agnostic
  example that scaffolded apps ship with — it defines a tiny custom plugin
  and exercises both mockPluginContext (route recording) and expectStream
  (ordered event assertions), running with no workspace or network.

Ships the kit to users, satisfying the plan's acceptance criteria that a
docs page exists and the template carries at least one example test.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Validation by scaffolding a real app with `databricks apps init` surfaced
that the examples called the `analytics()`/`toPlugin()` factory and then
treated the result as a plugin instance — but a factory returns a
{ plugin, config, name } descriptor for createApp to construct, so
`.attachContext`/handler methods are absent.

Rewrite both the template example test and the docs "Full example" to
instantiate the plugin class directly (`new GreeterPlugin({})`), matching how
the migrated agents suites use the kit. The scaffolded app's `npm test` and
`tsc` both pass against the published `@databricks/appkit/testing` subpath
with no workspace or network.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…pe error

Drop `undefined` from the static FakeToolValue union. `resolve()` treats an
undefined map entry as "unregistered tool" and throws, so allowing undefined
as a declared response made `{ query: undefined }` a confusing runtime error
instead of a compile error. A function returning undefined still works for the
rare "returns nothing" case. Add a test pinning that a null response is
returned as a value, not misread as a missing tool.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…ting kit

The plan's step 5 was to MOVE the fixtures into the package, not copy them.
The shipped kit (src/testing/fixtures.ts) duplicated all 15 exports of
tools/test-helpers.ts, which would drift over time. Collapse the original
into a thin re-export of @databricks/appkit/testing so src/testing is the
single source of truth while the 18 existing @tools/test-helpers importers
keep working unchanged.

The re-exported mockServiceContext is now synchronous; every call site either
awaits it (no-op on a non-promise) or reads it through
Awaited<ReturnType<...>>, so all suites pass unchanged (full appkit suite:
3117 passed, 1 pre-existing skip).

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…ing docs

Code review follow-ups:

- expectStream's parseSSEBody split frames on \n\n, so a spec-compliant SSE
  stream delimited by \r\n\r\n (from a real server) collapsed into one event.
  AppKit's own writer uses \n\n so existing tests were unaffected, but
  expectStream is public API that accepts any Response. Normalize CRLF to LF
  before splitting; add a CRLF regression test.
- Docs: instantiate the plugin CLASS in the attach() snippet (the factory
  returns a descriptor, not an instance), and note that the cache attach()
  seeds is a per-process singleton shared by tests within a file.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

📦 Bundle size report

Compared against bundle-size-baseline.json (main).

@databricks/appkit ⚠️ over budget

npm tarball (packed): 898 KB (+58 KB) — gzipped download (dist + bin; excludes release-only docs/NOTICE).

dist raw gzip
JS (runtime) 913 KB (+45 KB) 320 KB (+17 KB)
Type declarations 349 KB (+35 KB) 122 KB (+13 KB)
Source maps 1.8 MB (+99 KB) 602 KB (+36 KB)
Other 11 KB 3.7 KB
Total 3.0 MB (+178 KB) 1.0 MB (+67 KB)
Per-entry composition (own code — deps external (as shipped))
Entry Initial (gz) Lazy (gz) Total (gz) node_modules (min) Own code (min)
. 89 KB (+409 B) 2.5 KB 91 KB (+409 B) external 290 KB (+1.4 KB)
./beta 49 KB (+43 B) 457 B 49 KB (+43 B) external 143 KB (+168 B)
./testing 33 KB 29 KB 61 KB external 174 KB
./type-generator 21 KB 0 B 21 KB external 61 KB

Chunks:

Entry Chunk Load Size (gz)
. index.js initial 85 KB
. utils.js initial 4.0 KB
. remote-tunnel-manager.js lazy 2.5 KB
./beta beta.js initial 33 KB
./beta stream-manager.js initial 5.8 KB
./beta wide-event-emitter.js initial 3.2 KB
./beta databricks.js initial 3.0 KB
./beta configuration.js initial 2.1 KB
./beta service-context.js initial 1.3 KB
./beta client.js initial 434 B
./beta client-options.js initial 220 B
./beta supervisor-api.js lazy 192 B
./beta databricks.js lazy 142 B
./beta index.js lazy 123 B
./testing stream-manager.js initial 18 KB
./testing index.js initial 11 KB
./testing wide-event-emitter.js initial 2.9 KB
./testing index.js lazy 25 KB
./testing remote-tunnel-manager.js lazy 2.5 KB
./testing utils.js lazy 1.2 KB
./type-generator index.js initial 21 KB

@databricks/appkit-ui

npm tarball (packed): 342 KB (-291 B) — gzipped download (dist + bin; excludes release-only docs/NOTICE).

dist raw gzip
JS (runtime) 390 KB 130 KB (+1 B)
Type declarations 228 KB 83 KB
Source maps 752 KB (-334 B) 247 KB (-196 B)
CSS 16 KB (-462 B) 3.2 KB (-90 B)
Total 1.4 MB (-796 B) 464 KB (-285 B)
Per-entry composition (consumer bundle — deps bundled, peerDeps external)
Entry Initial (gz) Lazy (gz) Total (gz) node_modules (min) Own code (min)
./js 5.3 KB 49 KB 55 KB 208 KB 14 KB
./js/beta 20 B 0 B 20 B 0 B 0 B
./react 432 KB (+127 B) 49 KB 480 KB (+127 B) 1.3 MB 175 KB
./react/beta 1.0 KB 0 B 1.0 KB 0 B 1.9 KB

Chunks:

Entry Chunk Load Size (gz)
./js index.js initial 5.2 KB
./js chunk initial 120 B
./js apache-arrow lazy 49 KB
./js/beta beta.js initial 20 B
./react index.js initial 430 KB
./react tslib initial 2.1 KB
./react apache-arrow lazy 49 KB
./react/beta beta.js initial 1.0 KB

⚠️ Over budget: a package's shipped tarball, or a browser entry's consumer bundle (deps included), grew by more than 5% (and >10 KB). This check will fail — reduce the size, or acknowledge the increase by updating bundle-size-baseline.json.

@IamGalymzhan IamGalymzhan changed the title Feat/testing kit feat(test-kit) Aug 11, 2026
@IamGalymzhan IamGalymzhan changed the title feat(test-kit) feat(appkit): add testing kit Aug 11, 2026
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

🤖 AppKit PR bot

🔬 Run evals

Start an eval for this PR from the evals-monitor app: Go to Evals Monitor →

📦 Try this PR's app template

Scaffolds a new app from this PR's SDK build. Run it in any folder (requires the GitHub CLI — gh auth login — and the Databricks CLI):

gh run download 32148611814 -R databricks/appkit -n appkit-template-0.61.1-pr.2b30367-feat-testing-kit-530 -D appkit-pr-530 \
  && unzip -o "appkit-pr-530/appkit-template-0.61.1-pr.2b30367-feat-testing-kit-530.zip" -d "appkit-pr-530" \
  && databricks apps init --template "appkit-pr-530"

The template pins @databricks/appkit and @databricks/appkit-ui to tarballs built from this branch, so the scaffolded app runs against this PR's code.

IamGalymzhan and others added 7 commits August 11, 2026 11:28
CI's "Lint & Type Check" job runs `pnpm run check` over the whole repo, so a
pre-existing lint error unrelated to this branch failed the build:

- remote-tunnel-controller.test.ts had two `afterEach` hooks in one describe
  (lint/suspicious/noDuplicateTestHooks, error severity). Merge them into one —
  behavior preserved (env reset + console-spy clear both still run after each
  test). This file is byte-identical to main; the error predated the branch and
  only surfaced because CI lints the entire tree.

Also drop two dead `biome-ignore lint/suspicious/noExplicitAny` suppressions in
the testing kit (fixtures.ts, expect-stream.test.ts): `noExplicitAny` is turned
off repo-wide in biome.json, so the comments had no effect (suppressions/unused
warnings). The invalid-source test now casts through `unknown as never`.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Verified and fixed the findings from an independent code review:

- #1 (correctness) expectStream dropped the wire `event:` name when the JSON
  payload carried its own `type` (spread ran after the assignment). Spread the
  payload first, then set `type = name ?? parsed.type`, so a frame like
  `event: error` + `data: {"type":"result"}` reports `error`. Regression test added.
- #2 (contract) `@databricks/appkit/testing` eagerly loads vitest via fixtures
  even for `expectStream`, so vitest is a real requirement. Drop the "optional"
  peerDependenciesMeta and correct the docs sentence.
- #6 (OBO fidelity) the fake `asUser` recorded `asUser: true` unconditionally.
  Enforce the real `Plugin.asUser` token precondition: a request without
  `x-forwarded-access-token` throws `missingToken` (missing user id throws too),
  and the resolved `userId` is recorded on each tool call. Tests now assert both
  directions (well-formed request vs token-less).
- #3 (fidelity) attach() now mirrors AppKit core: registerPlugin plus
  registerToolProvider for real tool providers, without clobbering injected
  fakes. getPlugins()/getPluginNames()/hasPlugin() behave as in production.
- #12 unknown-tool lookup used `tools[name] === undefined`, so a tool named
  "constructor"/"toString" hit Object.prototype. Use Object.hasOwn.
- #5 drop data-less named SSE frames (real clients ignore them).
- #7 re-export the PluginContext type from the testing barrel so
  MockPluginContext.ctx is nameable through the exports map.
- #13 correct the docs: mock.telemetry captures the context's executeTool spans,
  not plugin-level spans (attachContext rebuilds the plugin's own telemetry).
- #4 parseSSEResponse now delegates to the same parseSSEBody as expectStream —
  one parser, no divergence. All 3 analytics.integration call sites still pass.
- #8 reformat template/server/example.test.ts with the template's Prettier so a
  scaffolded app's `npm run format` passes.
- #10 fix the package-doc @example (agentsPlugin._handleStream does not exist).
- #11 add kit tests that exercise attach() end-to-end (cache seed, isReady,
  registration, fake-not-clobbered).

Build passes attw + publint; full appkit suite 3125 passed / 1 pre-existing skip.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
… dep

With vitest declared as a (non-optional) peerDependency, knip recognizes it as
used, so the earlier ignoreDependencies entry is unnecessary. This reverts
knip.json to its original state.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
A required peerDependency has no per-subpath scope: it applied to the whole
@databricks/appkit package, so every production consumer that never imports
the testing kit got an unsatisfied peer (npm 7+ auto-installs vitest into
their tree; pnpm warns) — a wider blast radius than the eager-import bug it
was meant to fix.

Follow appkit's own precedent instead: `vite` backs the ./type-generator
subpath as a normal `dependency`, installed for everyone but loaded only by
importers of that subpath. Do the same for `vitest` and ./testing. vitest is
referenced solely by dist/testing/fixtures.js, never by the main/plugin/core
entry, so a consumer importing createApp never loads it.

Verified end-to-end: scaffolded an app whose own vitest (4.1.9) differs in
major from appkit's dependency (3.2.4), forcing a nested second copy. The
testing kit's vi.fn()/vi.spyOn() mocks and expect(...).toHaveBeenCalled()
assertions work across the two instances (vi spies carry their own call
state), and npm install emits no peer-dep warning. Build passes attw + publint.
Also fold in the template example's Prettier formatting (template uses Prettier,
not Biome) so a scaffolded app's `npm run format` passes.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The helper builds the REAL PluginContext with faked edges — it does not mock
the context — so the name was misleading. Rename to createTestPluginContext
(and the MockPluginContext type to TestPluginContext), matching the
create*-for-tests convention, and rename the files to test-plugin-context.ts.
Pre-merge and unreleased, so no external consumers are affected.

Also finish the #13 doc-accuracy fix in the shipped JSDoc (not just the docs
page): the telemetry field comment now states it captures the context's spans
(executeTool), not plugin-internal spans — attachContext rebuilds the plugin's
this.telemetry from the real TelemetryManager. These comments ship in
dist/testing/*.d.ts, so IntelliSense previously showed the unqualified claim.

Build passes attw + publint; full appkit suite 3125 passed / 1 pre-existing skip.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Behavior-preserving cleanups in the testing kit:

- createMockRequest reuses createMockWorkspaceClient() instead of an inline
  copy of the same mock client (verified identical).
- createMockServiceContext / createMockUserContext / mockServiceContext inline
  the createMockWorkspaceClient() call into the `||` fallback, so the mock
  client is built only when the caller did not supply one.
- The fake asUser view spreads `...base` and overrides executeAgentTool rather
  than re-declaring getAgentTools.
- expectStream's isSubsequence breaks once the expected sequence is fully
  matched.

No semantic change; typecheck clean and all kit + migrated tests pass.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
@IamGalymzhan
IamGalymzhan marked this pull request as ready for review August 12, 2026 12:40
@IamGalymzhan
IamGalymzhan requested a review from a team as a code owner August 12, 2026 12:40
- #1 (P1) The docs called vitest a peer dependency, but the manifest ships it
  under `dependencies` (the decision we landed on, matching how appkit ships
  `vite` for ./type-generator). Correct the docs to match: appkit installs
  vitest for you, and it loads only when you import ./testing. Manifest and
  docs now agree.
- #2 (P2) expectStream buffered the source eagerly with no bound, so a
  non-terminating stream hung until the runner's own timeout. Add an optional
  `{ timeout }` that fails fast with a clear, kit-specific error; document it
  and cover both directions with tests.
- #3 (P2) The fake asUser replicates asUser's token precondition but not the
  real dev-mode `DEV_OBO_FALLBACK_KEY` OTel marker (a module-private telemetry
  detail). Narrow the docs and JSDoc to say so and point users at the recorded
  asUser/userId fields instead of isDevOboFallback().

Build passes attw + publint; full appkit suite 3141 passed / 1 pre-existing skip.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Exercise @databricks/appkit/testing against real core plugins to validate it
beyond the two agent proof sites and produce usage references:

- analytics.kit.test.ts: cross-plugin executeTool via createTestPluginContext —
  OBO identity (asUser/userId), token-precondition rejection, and per-call
  timeout abort. Needs only the kit (no workspace/ServiceContext).
- genie.kit.test.ts: drives the real _handleSendMessage SSE stream and asserts
  event order with expectStream(...).toEmit(...).

Both add genuinely new coverage (streamed SSE order + OBO dispatch identity were
untested). Full appkit suite 3145 passed / 1 pre-existing skip.

Developer-experience notes (kit wins + friction, e.g. createMockResponse doesn't
compose with expectStream) captured in internal/ for the milestone review.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>

@pkosiec pkosiec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing kit - review

I tried this kit two ways: read it closely, then used it to write real tests for the analytics and genie plugins. Short version: I'd adopt it. Wrapping the real PluginContext and faking only the edges (telemetry, tool providers, routes) gives you things a hand-rolled { executeTool } stub can't - real timeout composition, the real asUser token check, real route recording. attach() and OBO both worked cleanly in a fresh test.

One gap stood out in real use, and a few smaller things are worth a look. Details are in the inline comments; here's the map:

Biggest lever

  • The kit tests generators well, but you can't assert a plugin's real SSE route - createMockResponse throws the written bytes away. Fixing this is the single change that makes plugin testing feel complete. (see comment on fixtures.ts)

Worth deciding before wide use

  • vitest as a plain runtime dependency ships a test framework into every production install. An optional peer avoids that. (see package.json)

Convenience

  • An obo option on createMockRequest (+ show it in the docs example).
  • A resetTestCache() so the documented cache advice is actually followable.

Polish

  • The shipped example test uses an any-cast instead of the documented attach().
  • Two service-context helpers differ only by a create prefix.
  • A useServiceContextMock() hook could remove the repeated beforeEach/afterEach block.

Comment thread packages/appkit/src/testing/fixtures.ts Outdated
}
return this;
}),
write: vi.fn(function (this: Any) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createMockResponse().write() throws the bytes away - it only flips headersSent. So when a plugin streams SSE from a route, there's no way to feed that output into expectStream. You're left scraping res.write.mock.calls and matching strings by hand.

Why it matters: most plugins stream from a route, not a bare generator. Today the kit tests generators well but can't assert what a real route emits. This was the #1 rough spot when I used the kit on the analytics plugin.

Suggestion: have createMockResponse collect the written chunks, then let expectStream read them directly:

await handler(req, res);
await expectStream(res).toEmit("warehouse_status", "result");

The cleanest shape is for expectStream to accept the mock response (it's the kit's own type) and parse its captured buffer as SSE. If you'd rather leave expectStream untouched, expose res.sseResponse() that returns a real Response and pass that.

One gotcha: don't hand expectStream the raw body string - a string is an iterable of characters, so it would be consumed one char at a time. It needs the mock response or a Response, not text.

Comment thread packages/appkit/package.json Outdated
"semver": "7.7.3",
"shared": "workspace:*",
"vite": "npm:rolldown-vite@7.1.14",
"vitest": "3.2.4",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vitest is a plain dependency, so it's installed into every app - including production installs that never import /testing. That pulls a whole test framework (and a second copy of vite) into code that never runs in production.

Why it matters: bigger installs, slower cold starts, and more supply-chain surface for nothing. The exact pin 3.2.4 can also clash with an app's own vitest, leaving two copies - and then the app's clearMocks/restoreMocks won't reset the spies this kit creates.

Heads-up: optionalDependencies would not fix this - npm and pnpm still install those by default (only --no-optional skips them).

Suggestion: make it an optional peer, and keep a dev copy for AppKit's own tests:

"peerDependencies":     { "vitest": ">=3" },
"peerDependenciesMeta": { "vitest": { "optional": true } },
"devDependencies":      { "vitest": "3.2.4" }

An optional peer isn't auto-installed, so it stays out of apps that don't test. Anyone who imports /testing is already writing vitest tests, so they have it - and there's only ever one vitest instance. (A devDependency alone won't work: it isn't installed for consumers, so their own tests couldn't resolve vitest.) Pick a range that covers the vitest majors you test against - the vi.fn/vi.spyOn APIs the kit uses are the same in v3 and v4.

Bigger alternative, if we'd rather /testing need no test framework at all: drop import { vi } from fixtures.ts and hand-roll the small spy surface it uses. More work - mockServiceContext relies on vi.spyOn(ServiceContext, ...) - so the optional peer is the smaller, idiomatic fix.

* WorkspaceClient (SQL succeeds, warehouse is RUNNING) on both the user and
* service-principal client slots; override any field via `overrides`.
*/
export function createMockRequest(overrides: Any = {}) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createMockRequest() ships with empty headers, so every on-behalf-of test has to hand-add the same two headers (x-forwarded-access-token, x-forwarded-user) to make asUser resolve. The cross-plugin example in the docs doesn't show them, so copying it throws Missing user token.

Why it matters: OBO is the kit's headline feature. The first thing a reader tries fails, and everyone repeats the same header boilerplate.

Suggestion: add an obo option to createMockRequest so OBO is one flag, not hand-rolled headers - it keeps a single entry point instead of a second function to discover:

createMockRequest({ obo: true });                  // default test user + token
createMockRequest({ obo: { userId: "alice" } });   // pick the user

It would set the two headers for you and still merge the other overrides. Then use it in the docs cross-plugin example. (A standalone createOboRequest(userId, token?) would also work, but the option keeps one request builder.)

Comment thread docs/docs/development/testing.md Outdated

Instantiate the plugin **class** directly (`new MyAgentPlugin(...)`). The `analytics()` / `agents()` factories you pass to `createApp` return a descriptor for the app to construct — for a unit test you want the instance.

The cache `attach()` seeds is a process-wide singleton: `CacheManager` is initialized once per test process and reused. Vitest isolates test *files* in separate workers, so caches never leak across files, but tests **within one file** share it. If a test populates the cache and a later test in the same file must not see it, reset between tests (e.g. clear the cache in `beforeEach`).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs note the cache is a shared singleton: isolated between test files, but shared by every test within one file, and vitest does not clear it between those tests. The kit ships no helper to clear it.

Why it matters: a value one test caches can leak into the next test in the same file - a stale read or a false pass. The docs say to "clear the cache in beforeEach", but there is no API to do it. A reset is also handy within a single test - e.g. to check a cache miss, then a hit.

Suggestion: export an async resetTestCache() that wraps CacheManager.getInstanceSync().clear() (and no-ops when the cache isn't initialized yet, since getInstanceSync() throws otherwise). Show it in the beforeEach here.

const mock = createTestPluginContext({ analytics: { query: "rows" } });
const executeToolSpy = vi.spyOn(mock.ctx, "executeTool");
// biome-ignore lint/suspicious/noExplicitAny: attach the real context to the plugin
(plugin as any).context = mock.ctx;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sets (plugin as any).context = mock.ctx instead of the documented await mock.attach(plugin).

Why it matters: this is the example people copy. The cast skips cache seeding, the isReady flip, and registry wiring - so pasted onto a plugin that reads this.cache, it breaks. In a fresh test attach() just works (I confirmed it while dogfooding).

Suggestion: use attach() here. If you genuinely need to skip the wiring, add a small documented setter so the shipped pattern isn't an any-cast.

Comment thread packages/appkit/src/testing/fixtures.ts Outdated
* Builds a {@link ServiceContextState} for testing without touching the
* singleton. Use with {@link mockServiceContext} to install it.
*/
export function createMockServiceContext(options: TestContextOptions = {}) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createMockServiceContext builds a value and installs nothing; mockServiceContext installs spies and returns restore(). They sit next to each other and differ only by the create prefix.

Why it matters: in autocomplete they look interchangeable. Pick the builder by mistake and the real singleton stays live, so the test fails in a confusing way.

Suggestion: rename to say what each does (e.g. buildServiceContextState vs installServiceContextSpies), or drop the unused builder from the public exports. If you add useServiceContextMock() (see the other comment on this file), these two become its lower-level building blocks - name them so that reads clearly. Renaming is free now, while the kit is new.

// `Object.hasOwn`, not `tools[toolName] === undefined`: a tool named
// "constructor"/"toString"/etc. would otherwise resolve to an inherited
// Object.prototype method and be invoked instead of reported missing.
if (!Object.hasOwn(tools, toolName)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three edges the kit deliberately handles have no test pinning them:

  • the Object.hasOwn guard here (a tool named constructor/toString must report "missing", not resolve to a prototype method),
  • the dev-mode branch of the fake asUser (test-plugin-context.ts:277), which never runs because the suite is under NODE_ENV=test,
  • the scalar / array / non-JSON / multi-line data: paths in parseSSEBody (expect-stream.ts:80).

Why it matters: each is a corner someone could quietly regress - the guard could be weakened to in/=== undefined and CI would stay green.

Suggestion: add one small case for each (a constructor tool name rejects; a NODE_ENV=development token-less request is allowed through; a data: 42 / data: [1] / data: plain text frame parses as expected).

*
* @returns The mock context plus the spies and a `restore()` helper.
*/
export function mockServiceContext(options: TestContextOptions = {}) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every suite that mocks the service context repeats the same three steps: call mockServiceContext() in beforeEach, hold the handle, and call restore() in afterEach.

Why it matters: it's boilerplate in every service-context test, and forgetting the restore() leaks spies into the next test - a confusing, order-dependent failure.

Suggestion (small enough to add here): ship a useServiceContextMock(options?) that registers the setup and teardown for you:

useServiceContextMock();   // one line: auto beforeEach setup + afterEach restore

It just wraps the existing mockServiceContext and calls vitest's beforeEach/afterEach, so it's a thin helper, not a rewrite. (This also leans on vitest being present - one more reason to settle it as the optional peer from the package.json comment.)

Two things to get right so it's actually usable: call it at the top of a describe, not inside a test (beforeEach/afterEach only register during collection); and return a live accessor, not the handle itself - each beforeEach builds fresh spies, so a returned snapshot goes stale. Hand back something like { get current() { ... } } so a test can still assert on the spies.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be placed under plugins docs? as "Testing plugins". What do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but name Testing seems fine then to not make it duplicate

@pkosiec pkosiec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2 - the new dogfood tests

Thanks for adding these. Both pass, and the analytics one adds real coverage that was missing (cross-plugin executeTool OBO identity).

Two asks:

  1. Don't keep them as separate *.kit.test.ts files. The testing kit should be the default way to test a plugin, not a parallel track. Fold these into analytics.test.ts / genie.test.ts and drop the .kit split. For genie, that also means merging with the existing (presence-only) SSE test into one ordered assertion on the real event names.
  2. Trim the comments. Both files carry a lot of explanatory narration that reads as slop; a /deslop pass when merging would cut it to what's needed.

Bonus signal: these files independently re-hit round 1 - a hand-rolled capturing response (C1), a re-invented service-context mock (C8), and hand-rolled OBO headers (C3). Two authors needing the same workarounds is the strongest case yet to land those kit ergonomics.

import { describe, expect, test } from "vitest";
import { createTestPluginContext } from "../../../testing";

/**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good tests, but two things about the file itself.

Don't keep them in a separate analytics.kit.test.ts. The kit should be the default way to test a plugin, not a parallel "kit" track. A separate file signals the kit is optional, splits the plugin's suite in two, and leaves it unclear where a new test should go. Fold these three tests into analytics.test.ts and delete this file - the coverage (cross-plugin executeTool OBO identity) is genuinely new, so keep it, just not as its own file.

Trim the narration. The docblock and inline comments explain the exercise at length; it reads as slop. A /deslop pass when you merge would cut it to the essentials.

Why it matters: if this is how we want people to test, the tests should look like normal plugin tests - same file, minimal ceremony - not a heavily annotated "kit demo".

import { createTestPluginContext, expectStream } from "../../../testing";
import { type GeniePlugin, genie } from "../genie";

/**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the analytics file: merge this into genie.test.ts rather than keep a separate genie.kit.test.ts, and trim the heavy comment narration (/deslop) while you do.

One extra reason to merge: genie.test.ts already tests this SSE path ("should call startConversation and stream SSE events"), but only checks event presence via mockRes.write.mock.calls.join("").toContain(...) - no ordering, brittle substrings. This file adds ordering via expectStream. Two tests for one path is worse than one good one.

Suggestion: replace that existing test's assertion block with expectStream(...).toEmit(...) on genie's real event names, and delete this file.

* kit's own `createMockResponse` throws written chunks away, so streaming
* handler tests need this until the kit ships a capturing response.
*/
function createCapturingResponse() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For whoever lands the kit changes: this local createCapturingResponse is exactly the createMockResponse gap from the first review (C1), now hand-rolled a second time. Two dogfood files needing the same bridge is the signal to ship a capturing response in the kit and drop these local copies.


// A minimal ServiceContext stand-in (the kit's mockServiceContext fixture
// covers this, but genie's streaming path only needs a resolvable context).
function mockServiceContextLite() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also re-invents mockServiceContext plus a manual restore - the boilerplate the proposed useServiceContextMock() (C8) removes. When this folds into genie.test.ts, use the kit fixture instead of a local stub.

streamSendMessage: (...a: unknown[]) => AsyncGenerator<unknown>;
};
}
).genieConnector.streamSendMessage = async function* () {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you merge this in, assert genie's real SSE event names. The fake connector here yields status/message/complete, but genie actually emits message_start/status/message_result/query_result (see the existing test). Ordering on invented names won't catch a reorder or drop of the real ones.

* workspace-client fixtures because that work lives behind those seams.)
*/

function mockReq(headers: Record<string, string>): express.Request {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: another hand-rolled OBO request with the two headers - more evidence for a createMockRequest({ obo }) option (C3).

Resolve the eight review comments on the testing kit:

- createMockResponse now captures written SSE bytes and exposes
  sseResponse(); expectStream reads a captured mock response directly, so
  streaming-route tests no longer need a hand-rolled bridge.
- Ship vitest as an optional peer dependency (+ devDependency) instead of a
  plain runtime dependency, keeping the test framework out of production
  installs and deduping to the app's own copy. Ignore it in knip.
- Add an obo option to createMockRequest so on-behalf-of tests set the
  forwarded identity headers with one flag.
- Add resetTestCache() to clear the shared cache singleton between tests.
- Use the documented attach() instead of an any-cast in the agents
  dispatch tests.
- Drop the unused createMockServiceContext/createMockUserContext builders
  from the public surface; keep the service-context builder internal.
- Pin the previously untested edges: the Object.hasOwn tool-lookup guard,
  the dev-mode asUser branch, and parseSSEBody's non-object data values.
- Add useServiceContextMock() to register the mock lifecycle in one line,
  returning a live accessor.

Dogfood the new helpers in the analytics, genie, and serving suites, and
document them in the testing guide.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The testing kit is entirely plugin-scoped (createTestPluginContext,
attach(plugin), plugin route/tool/SSE assertions), and the page's own
cross-links already pointed into plugins/. Move it next to custom-plugins
and fix the relative links. Keep the heading as 'Testing'; the Plugins
section supplies the context.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Address round-2 review: the kit should be the default way to test a
plugin, not a parallel '*.kit.test.ts' track.

- Fold the three cross-plugin executeTool OBO tests into analytics.test.ts
  and delete analytics.kit.test.ts.
- Upgrade genie.test.ts's SSE test to assert event ORDER via
  expectStream on genie's real event names (message_start, status,
  message_result, query_result), replacing brittle write.mock.calls
  substring checks, and delete genie.kit.test.ts.
- Trim the heavy comment narration from the folded-in tests.
- Re-export createTestPluginContext and expectStream from the test-helpers
  shim.
- Finish the testing-guide move under plugins/ (sidebar position + links).

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The dogfood fold trimmed expect(mock.toolCalls).toHaveLength(1), so a
double-dispatch would no longer fail the happy-path test — and it was
inconsistent with the token-less sibling that kept toHaveLength(0).
Restore it.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The toEmit swap pinned event order but dropped the payload values the old
substring checks covered (conversationId=new-conv-id, status=ASKING_AI),
which aren't asserted elsewhere. Restore them structurally via collect() +
toMatchObject — keeping the ordering guarantee without brittle substrings.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…equest

createMockRequest returned userWorkspaceClient, serviceWorkspaceClient,
getWarehouseId and getWorkspaceId — fields no production code reads
(plugins resolve those through getWorkspaceClient()/getWarehouseId() from
src/context, which mockServiceContext stands in for). Publishing them via
@databricks/appkit/testing would make four inert fields a permanent public
promise.

The two warehouse cold-start tests (analytics + metric) overrode
mockReq.serviceWorkspaceClient.warehouses.get, which the route never reads
— so they passed on the default RUNNING client without exercising the
warehouse path at all. Route the warehouse client through
mockServiceContext (the real seam) so the tests are live, and drop the
'mock WorkspaceClient' claim from the testing guide.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
@IamGalymzhan

Copy link
Copy Markdown
Collaborator Author

@pkosiec All comments are fixed

Brings feat/testing-kit up to v0.60.0. Three conflicts, all resolved in favour
of this branch:

- tools/test-helpers.ts — main only reformatted the old implementation; this
  branch replaced it with a re-export shim over packages/appkit/src/testing.
- agents/tests/route-handler-errors.test.ts and dispatch-tool-call.test.ts —
  this branch migrated both onto createTestPluginContext, a superset of main's
  raw-stub versions (dispatch-tool-call keeps an extra timeout-abort test).

Main migrated Biome -> oxlint+oxfmt, so this commit also reconciles the branch
with the new toolchain: the 64 dead biome-ignore comments are dropped from the
two conflicted test files, and the shipped testing-kit sources are reformatted
under oxfmt's import grouping.

Ignore **/.claude in knip, oxlint, and oxfmt. Agent worktrees live under
.claude/worktrees/, so every tool was analysing a second full copy of the repo:
knip reported hundreds of phantom unused exports and failed the pre-commit
hook outright, and a repo-root `oxfmt` would have rewritten another branch's
working tree.

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Every core plugin's actual work runs through getWorkspaceClient(), which the
testing kit did not fake — so a jobs/genie/serving/files plugin crashed on its
first client call and authors hand-rolled nested client literals instead.

createMockWorkspaceClient() fakes the whole facade in three layers:

- The 9 facade members are explicitly typed, so `client.jbos` is a compile
  error. The facade is closed and AppKit-owned, so there is no per-service
  fixture to maintain as the SDK grows.
- Each service is a Proxy minting one memoized vi.fn() per method name, keyed
  by dotted path. `client.jobs.getRun === client.jobs.getRun`, so call
  assertions work, and the legacy view shares the map so one `responses` entry
  covers both — including un-faceted services like `legacy.clusters.list()`.
- `config` and `apiClient` are seeded objects rather than bare Proxies, because
  three of their members must not be mocks: `config.host` is a real string that
  production code builds URLs from and throws on when falsy,
  `apiClient.userAgent()` must be synchronous (a Promise inside a Headers value
  stringifies to "[object Promise]"), and `apiClient.request` resolves {} so
  destructuring its result does not throw.

Two guards keep the Proxy safe. Symbol keys delegate to Reflect.get, and a
passthrough deny-set answers `undefined`. `then` is the load-bearing entry:
without it a service looks thenable, so `await client.jobs` either hangs or
resolves to a mock's return value. ownKeys is left at its default so
util.inspect and toEqual see {} instead of recursing forever.

The three historical canned defaults are byte-identical, because 13 test files
reach them implicitly through mockServiceContext. `currentUser.me` is additive
and load-bearing: ServiceContext.createContext reads `currentUser.id`, so an
unresolved me() is a TypeError and createApp({ client }) cannot boot without it.

getMockFn(client, "jobs.getRun") is the typed assertion path — facade accessors
are legacy-SDK-typed, so expect(client.jobs.getRun).toHaveBeenCalled() does not
typecheck. It mints idempotently, so the handle can be grabbed before the code
under test runs.

The compile-time block is enforced by tsc, not at runtime. It records one
correction to the plan: the SDK types `config.host` as `string | undefined`, so
the contract is that it narrows to a string, not that it is non-optional.

4451 tests pass (+38); the 667 tests reaching the default client indirectly
through mockServiceContext are unchanged.

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
fixtures.ts had its own two-service createMockWorkspaceClient, so the shipped
fixture and the new never-crash builder were near-duplicates. The fixture now
re-exports the builder and the barrel points at its new home.

The blast radius is entirely indirect. Nothing in src imports the exported
fixture by name (connectors/genie/tests/client.test.ts defines its own local
one), but buildServiceContextState calls it as the default client for
mockServiceContext, which 13 test files use. The risk therefore lives in the
default return value, which is why U1 kept the three canned defaults
byte-identical — and why this commit adds the convergence guard that asserts
both halves: jobs/genie now resolve instead of throwing "Cannot read properties
of undefined", while the SQL path those 13 files depend on still succeeds.

createConfigurableMockWorkspaceClient is left byte-for-byte unchanged and only
gains a @deprecated notice. Its bare vi.fn()s return undefined *synchronously*
whereas the new floor returns Promise<undefined>, and its one caller
(analytics.integration.test.ts) can observe that difference; reimplementing it
here would change behaviour for no benefit. It migrates with that suite later.

The jobs suite drops its hand-rolled client literal — the seven method mocks
plus the config.host/authenticate block — onto the builder, which is the proof
the boilerplate actually goes away. Its 57 assertion sites move to a getMockFn
handle because facade accessors are legacy-SDK-typed, so .mockResolvedValue on
them does not typecheck. The factory needs `await vi.hoisted(async ...)` with a
dynamic import, since a hoisted factory runs before the file's imports.

4454 tests pass (+3).

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
LifecycleManager's shutdown sequence was reachable only by killing the process,
so nothing could release AppKit's sockets, timers, pools, cache, and telemetry
and keep running. That is what blocks an app handle's close(), and with it any
test that wants to boot more than once in a file.

The sequence is now a phase runner that returns an exit code, a promise memo,
and two thin callers:

- shutdown() is the signal path, observably unchanged: it arms the same
  unref'd 15s force-exit backstop and still exits 0 on completion, 1 on an
  unexpected throw. The timer stays here deliberately — it is the one thing
  close() must not inherit, since a programmatic caller wants a logged error
  when teardown hangs, not a dead process.
- close() is the programmatic path: it detaches signal handlers, runs the same
  phases under a shorter default budget (5s, not the production 15s), logs the
  phase that was in flight if the budget is spent, and never exits.

Replacing the isShuttingDown boolean with a promise memo is a strict
improvement. The boolean made a second caller return *immediately* while
teardown was still running — harmless for a signal, since the first caller
exits the process anyway, but for close() it would resolve before resources
were released, which is the difference between a correct handle and a
misleading one. The read and the assignment stay in one synchronous statement,
preserving the invariant the boolean was there to protect. One production
behaviour does shift: a second signal now awaits the first teardown.

installSignalHandlers registered anonymous arrows that could never be removed.
The [signal, handler] pairs are now retained and detached individually, never
via removeAllListeners, so a host embedding AppKit keeps its own handlers. The
tests assert that with two managers installed, a.close() leaves b's pair and an
unrelated host listener intact, and that counts return to their pre-install
baseline — which is what stops repeated boots tripping
MaxListenersExceededWarning.

The signal-mid-close race is documented rather than papered over: handlers come
off before the first await, and if a signal still lands it joins the memo and
exits, because it wanted the process dead.

The idempotency test is verified by injection — it fails against the old
return-immediately semantics and passes against the memo. Its first draft did
not: it counted microtask ticks, which cannot distinguish an early return
through close()'s raceWithTimeout wrapper. It now asserts that neither caller
settles until the plugin hook has actually completed.

4463 tests pass (+9); the 14 pre-existing shutdown tests are untouched.

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
createApp acquired sockets, timers, and pools but returned no way to release
them, so the only teardown was killing the process. The LifecycleManager built
at the end of _createApp was constructed and immediately discarded; it is now
retained on the instance and reachable through the handle.

The return type widens from PluginMap<T> to AppHandle<T>, which is PluginMap<T>
plus close() and Symbol.asyncDispose. Widening a return type is
source-compatible for every existing caller, and the cast that produces the
handle already hid instance methods, so close() rides along naturally.
onPluginsReady deliberately keeps PluginMap<T>: it runs before the server
starts, so handing it a close() would invite a footgun for no gain.

The name collision is a real hazard, not a theoretical one. Plugin exports are
installed with Object.defineProperty, and an own property shadows a prototype
method — so a plugin named `close` would silently replace teardown rather than
merely confuse the types. Three layers guard it: Symbol.asyncDispose is
unreachable from a manifest name, so `await using` is always safe;
createAndRegisterPlugin now throws a ConfigurationError naming the offending
plugin; and no plugin in the repo is affected.

Coverage is deliberately unmocked, because the claim is about real resources: a
boot on an ephemeral port serves /health, close() runs the plugin's shutdown
hook, the socket stops accepting, and the SIGTERM listener count returns to its
pre-boot baseline. Also covered: idempotency at the app level, a server-less app
closing cleanly, `await using` releasing at scope exit, and the reserved name
being rejected. Verified by injection — with close() stubbed to a no-op and the
reserved-name guard removed, 5 of the 6 fail.

4469 tests pass (+6).

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
close() released resources but left the singletons pointing at them, so
close() followed by createApp() silently reused what the teardown had just
torn down. This delivers the actual driver — boot, assert, close, repeat.

CacheManager.reset() drops both `instance` and `initPromise`. Clearing only
`instance` is insufficient because getInstance() returns `initPromise` when
`instance` is null, so the next boot would await a promise resolving to the dead
manager. Testing surfaced a third case the plan missed: clearing both is *still*
not enough, because an initialization already in flight runs its continuation
and re-publishes the very instance being discarded. A generation counter now
invalidates that write.

The covering test models PersistentStorage rather than using the default
in-memory storage. This matters: InMemoryStorage.close() only clears a Map and
stays usable, so an in-memory test passes whether or not the reset exists —
which is precisely why the bug hid. Against storage whose close() is terminal,
the way pool.end() is, the test shows the stale manager throwing "Cannot use a
pool after calling end()" and the reset fixing it.

One plan claim is corrected rather than implemented. The plan asserted that
TelemetryManager's never-cleared `shutdownPromise` made a second shutdown()
return a stale promise and skip flushing a re-initialized SDK. It does not:
shutdown() only returns the memo after reassigning it for whatever SDK is
currently live, so a stale resolved promise can be returned only when there is
no SDK to flush. Verified twice — by mocking NodeSDK across three
initialize/shutdown cycles, and by running the original implementation in
isolation. An earlier draft of this commit added a generation counter here too;
it has been reverted, since it fixed nothing and cost a field. What TelemetryManager
did need, and now has, is the static reset() that drops the singleton.

The resets are wired into close() only, never the signal path, where the process
is dying and pointer drops are pure cost. Symmetry is the justification: core
initializes all four in _createApp, so core drops all four. This is a semantic
expansion, not purely a bug fix — a host that closes and then expects
ServiceContext.get() to work will now get an InitializationError.

resetAppKitSingletons() is published from @databricks/appkit/testing for tests
that hand-roll createApp and would otherwise deep-import
../context/service-context to reach ServiceContext.reset(). Both it and
LifecycleManager.close() delegate to one core-side implementation rather than
duplicating the list. resetTestCache() is untouched — it calls clear() on the
existing cache, a different and still-useful operation.

4480 tests pass (+11).

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
One call boots a real AppKit app with no workspace, no credentials, and no
network, and calls it over real HTTP:

  const app = await createTestApp({ plugins: [myPlugin()] });
  const res = await app.post("/api/my-plugin/thing", { body, obo: true });
  await expectStream(res).toEmit("status", "result");
  await app.close();

Four of the setup steps exist only because of hazards found by reading the boot
path, and each has a test that fails without it:

- NODE_ENV is pinned away from "development". Not tidiness: dev mode routes the
  injected `port: 0` through get-port, where portNumbers(0, …) throws a
  RangeError. "development" is refused outright with an explanation rather than
  worked around, since dev mode also boots a real Vite server, downgrades
  resource validation to a warning, and stops filtering dev-only plugins.
- DATABRICKS_WORKSPACE_ID is set, short-circuiting the SCIM probe in
  getWorkspaceId, and internal telemetry is disabled. Both would otherwise fire
  apiClient.request during boot. A canary test asserts zero calls after boot, so
  either regression fails loudly.
- The cache gets explicit in-memory storage. Without it CacheManager builds its
  own workspace client — ignoring the injected one — and probes Lakebase over
  the network, so "no network" would be false.
- The server plugin is reached through a lazy `await import()`, because it runs
  dotenv.config() at module load. A static import would mutate a consumer's
  process.env merely by importing the testing entry point.

process.env is snapshotted wholesale rather than by whitelist, since plugins
read vars the harness cannot enumerate, and restored on close() — including
deleting keys the harness added and restoring a pre-existing DATABRICKS_HOST to
its own value rather than the test default. Teardown also runs from the
boot-failure path, or a plugin whose setup() throws would leak env mutations into
every later test in the file.

Plugin exports live under app.plugins rather than spread onto the handle: `get`
and `delete` are plausible plugin names and would collide with the request
methods.

The request methods return a native Response, so expectStream composes with no
bridge — the dogfooding report's top friction, avoided by construction. `obo`
reuses createMockRequest's OboOption rather than inventing a second convention.

Two corrections to the plan, both found by testing:

- A `strictValidation: false` opt-out was specified and has been dropped as a
  false affordance. enforceValidation computes `shouldThrow = !isDevelopment ||
  strict`, so with NODE_ENV pinned away from "development" validation always
  throws and the flag cannot do anything. The env var is still set as
  belt-and-braces, and a test pins the unconditional behaviour.
- The error-middleware test initially asserted a redacted body. It is not
  redacted: errorHandlerMiddleware hides the message only under
  NODE_ENV=production, and the harness pins "test". Useful for tests — an
  assertion can name the failure — but it means that response is the dev shape,
  which the test now says out loud.

The HTTP suite's probe plugin registers routes through `this.route()`, the way
real plugins do. Registered with raw `router.get()` a rejection escapes
forwardAsyncErrors and hangs the request — correct AppKit behaviour, and worth
having a representative test rather than a misleading one.

4511 tests pass (+31).

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…dogfood both

Audit first: the ./testing subpath was already in both exports maps and the
tsdown entry, vitest is already an optional peer dep, and attw/publint pass. The
real gap was proof that a test needs nothing else, so the integration suites
moved onto the public entry point — that migration *is* the audit. A new
acceptance suite imports only from @databricks/appkit/testing and boots,
requests, asserts a stream, and closes.

Self-referencing the package from inside it needed a tsconfig paths entry.
Resolving the package's own export map made tsc's project root ambiguous
(TS2209), and the alias mirrors how shared and @databricks/lakebase are already
mapped. It also makes source resolution deterministic rather than depending on
the "development" export condition — verified by marker: the subpath resolves to
src, not dist.

createTestPlugin(factory, config) closes the last dogfooding footgun. Reaching
through a descriptor with `new (genie({}).plugin)(config)` skips DEFAULT_CONFIG
and forgets `name`, so the instance under test is configured differently from
the one production builds. It mirrors createAndRegisterPlugin's merge order.
createTestApp does not subsume it: the harness takes descriptors and builds
instances itself, so the unit path needs its own ergonomics.

Dogfooding results, reported as measured rather than as hoped:

- analytics.integration.test.ts: 300 -> 216 lines. Setup/teardown went 104 -> 55,
  against the plan's predicted ~30. Its local getListeningPort helper is gone and
  its 12 mock handles now come from getMockFn. Same 6 tests, same assertions.
- getListeningPort is lifted into the kit, and files/plugin.integration.test.ts
  imports it instead of carrying its own copy.
- server.integration.test.ts moves four of its five blocks to ephemeral ports.
  The fifth keeps its fixed port deliberately, because it asserts the server
  honours a configured one; a comment says so. The removed sleep-100ms waits are
  replaced by getListeningPort, which waits on the listening event instead of
  guessing.

One plan claim corrected: the hardcoded TEST_PORT = 9879 said to collide with
server.integration was already fixed on this branch — analytics had moved to
port: 0. The real fixed ports were the five in server.integration itself, which
is what this commit addresses instead.

Docs lead with createTestApp: a which-harness comparison table, the dotted-path
responses convention, the teardown contract, a "Mocking Databricks services"
section carrying the never-crash floor's honest catch (a misspelled *method*
returns undefined, and a Lakebase pool built on the fake cannot connect), and an
explicit callout that manifest.config.schema is not validated. The
PluginContext/ServiceContext boundary note now says the kit covers the data
plane. The template example gains a createTestApp test.

4519 tests pass (+8). pnpm docs:build is clean.

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…ings

Verified the kit end to end the way the plan prescribes: pnpm pack:sdk, an app
scaffolded by `databricks apps init` from this repo's template, the tarballs
installed into it, and the suites run with no .env, no credentials, and every
non-loopback socket connection hard-blocked. Nine customer-style tests plus the
template's three pass, and the scaffolded app typechecks against the shipped
.d.ts.

That run corrected a claim this branch had been making. Both the plan's risk
table and the docs said a misspelled *method* slips through the never-crash
floor and only a misspelled *service* is caught. Not so: each facade accessor is
typed against the SDK's own service class, so `client.jobs.getRunz` and
`client.files.anything` are compile errors too. The compile-time block now
asserts that for three services, and the docs say what the real gap is — a
method that exists but has no declared response, or a call that bypasses the
types with a cast.

Also repointed one doc line that told readers to reach the client via
`getWorkspaceClient()`. That is right inside this repo but wrong from the
published entry, where the name currently resolves to Lakebase's unrelated
`getWorkspaceClient(config)`. The docs now use `getExecutionContext().client`,
which is exported and works. The mis-export itself is a main-entry defect,
outside this branch's scope, and is left for a follow-up.

4519 tests pass. Build, docs:build, attw, and publint are clean, and the packed
tarball carries dist/testing/*.js and .d.ts for every new module.

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Eight reviewers over the branch diff produced 24 findings; 14 were actionable.

The two real defects, both in code this branch added:

- **An orphaned teardown could tear down the *next* app's resources.**
  `close()` races the shutdown phases against a 5s budget, then resets the core
  singletons and resolves. The phases keep running. A plugin `shutdown()` hook
  slower than that budget but inside its own 10s per-plugin budget — which the
  files plugin's drain can be — left phase 5 re-reading the static slots, so it
  either skipped draining this app's cache pool or closed the *following* app's
  storage and shut down its OTEL SDK. The comment claiming the phases had
  "already closed the cache storage" was only true when teardown finished in
  budget. Phase 5 now uses the instances captured before the first await, and a
  test drives the exact 5s-to-10s window (verified by reintroducing the bug).

- **`process.env` restore did not compose across overlapping boots.** Each app
  snapshotted independently, so a second boot captured the first's mutations and
  whichever closed last re-applied them, stranding the harness keys and the first
  app's `env` entries after both apps were gone. Confirmed by probe, in-repo and
  from the packed tarball. There is now one reference-counted baseline: the first
  live app anchors it, the last one to close restores it, and the outcome no
  longer depends on close order.

Also fixed: `server: false` alongside a caller-supplied server plugin is now
refused instead of half-honoured (the plugin still bound a socket while the
handle denied one existed); `AppHandle.close()` declares the `{ timeoutMs }` the
implementation accepts, so the harness no longer casts to reach it; the duplicate
`listeningPort` helper in the close integration suite is gone in favour of the
kit's (two reviewers flagged it); the analytics suite drops an `as never` that
erased `app.plugins` typing; the `clientFns` WeakMap moved above its users; and a
comment claiming "9 typed facade members" over a 7-element array is corrected.

Two of my own tests were weak and are now stronger: the `authenticate` test
wrapped its whole body in `if (mockFn)` and only asserted "was called" — it now
asserts the Authorization header it claims to set — and a close-after-signal test
proved ordering by counting microtask ticks, which cannot see through
`raceWithTimeout`; it uses the same sentinel the sibling test does. A new
compile-time assertion pins that `AppHandle` still satisfies a `PluginMap`
annotation, so a regression in the widening can't pass silently.

Documented rather than changed: a service's methods are callable but not
enumerable, so `'getRun' in client.jobs` is false and `Object.keys` is empty.
Reporting those keys would make `util.inspect` mint a mock per probe, which is
the recursion the default traps exist to avoid. Also documented why
`onPluginsReady` keeps the narrower `PluginMap<T>`.

One finding rejected as a false positive: project-standards reported CLAUDE.md
still documents Biome. It does not — main's own oxlint migration (9538d58)
updated it, and only the pre-merge copy said Biome.

Six findings were demoted to residual risks, chiefly the P1 claim that the mock
resolving `undefined` for undeclared paths lets a test pass while production is
broken. That is the deliberate, documented contract of the never-crash floor, not
a defect; an independent reviewer re-deriving it argues the existing caution
callout is warranted, not that the design changed.

4524 tests pass. Re-verified end to end from a repacked tarball in the
`databricks apps init` app with all non-loopback sockets blocked.

Co-authored-by: Isaac
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`createApp()` rejects a plugin whose manifest name is `close`, because plugin
exports are installed as own properties and an own property shadows a prototype
method — so such a plugin would silently replace the app handle's teardown.

The thrown ConfigurationError already names the offending plugin, but nothing
told an author the constraint existed before they hit it. Noted beside where
custom-plugins.md introduces `static manifest`.

Landing this as part of the `feat:` framing for the branch rather than a
BREAKING CHANGE footer: the failure is loud and at boot, not a silent runtime
change, and no plugin in this repo is affected.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
These were added earlier on this branch to work around a locked agent worktree
at .claude/worktrees/, which every tool saw as a second full copy of the repo:
knip reported hundreds of phantom unused exports and failed the pre-commit hook
outright, and a repo-root oxfmt would have rewritten that other branch's tree.

The worktree has since been removed, so the ignores are treating a symptom that
no longer exists and are out of scope for this branch. Verified after removal:
`pnpm knip` and `pnpm check` both exit 0 at the repo root.

.oxfmtrc.json and .oxlintrc.json are now byte-identical to origin/main. The one
remaining knip.json difference — ignoreDependencies: ["vitest"] for
packages/appkit — predates this work and is required because vitest is an
optional peer dependency of the published testing subpath.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The comments on this branch were far past the repo's own density: reset.ts was
85% comment (35 of 41 lines) for a one-line function, create-test-plugin.ts 63%,
lifecycle-manager.ts 48%, mock-workspace-client.ts 45%. Much of it restated the
code or ran to several paragraphs where a clause would do.

Net 364 comment lines removed. Every file now sits at or below the repo baseline
(main's own sources run 20-42%): mock-workspace-client 45% -> 21%,
create-test-app 36% -> 25%, lifecycle-manager 48% -> 35%, reset.ts 41 -> 13 lines
total, create-test-plugin 63 -> 27.

What was kept is the non-obvious "why" that a maintainer would otherwise delete
and reintroduce a bug: that `then` must stay in the deny-set or `await
client.jobs` hangs; that `ownKeys` stays default or util.inspect mints a mock per
probe; that config.host must be a real string; that the three canned defaults are
byte-identical because 13 suites depend on them; that phase 5 captures its
singletons before the first await; and the four boot hazards behind
createTestApp's setup. Pre-existing comments in files this branch only touched
(fixtures.ts, test-plugin-context.ts, the shutdown() phase list) are left alone —
reverting other people's prose is not this change's business.

Also dropped an unnecessary `as Any` cast in createTestPlugin: DEFAULT_CONFIG is
already declared on PluginConstructor, so the type escape and its explanatory
comment both went.

4524 tests pass; lint, format, and typecheck clean.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The mock-workspace-client suite had 37 tests written against the plan's checklist
rather than against behaviours, so eight asserted something a sibling already
covered: getRun memoization twice, config.host being a string twice, the 9-member
facade twice (one a strict subset of the other), a rejecting function response
twice, getMockFn path resolution twice, function-valued responses twice, the
canned defaults twice, and two config-option tests that fit in one.

29 tests now, with no assertion lost — where a dropped test had a unique claim it
was folded into the survivor. Three describe blocks became empty and were
removed; one had only a comment saying its subject could not be tested at
runtime, which the compile-time contract block covers properly.

Note for anyone reading this as a bundle-size fix: it is not one. Tests do not
ship — the packed tarball contains zero test files — and dropping these eight
moved the measured bundle by exactly 0 bytes. The comment trimming in the
previous commit is what actually helped (+8.1% -> +6.9%), because JSDoc is
preserved in the emitted .d.ts.

4516 tests pass.

Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants